fix: handle changed urls #3
Conversation
files renamed to just config.json
WalkthroughThe changes in this pull request focus on the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
ovos_tts_plugin_nos/__init__.py (3)
Line range hint
73-79: Add security measures for file downloadsThe current implementation downloads files without proper security measures. Consider these improvements:
- Validate URLs against an allowlist
- Implement checksum verification for downloaded files
- Add proper error handling for file operations
Here's a suggested implementation:
@classmethod def get_engine(cls, model_path: str, config_path: Optional[str] = None) -> Synthesizer: - config_path = config_path or model_path.replace("celtia.pth", "config.json").replace("sabela.pth", "config.json") + ALLOWED_DOMAINS = ["huggingface.co"] + MODEL_CHECKSUMS = { + "celtia.pth": "<expected_checksum>", + "sabela.pth": "<expected_checksum>" + } + + def validate_url(url: str) -> bool: + from urllib.parse import urlparse + return any(domain in urlparse(url).netloc for domain in ALLOWED_DOMAINS) + + def get_config_path(model_path: str) -> str: + base_path = os.path.dirname(model_path) + return os.path.join(base_path, "config.json") + + config_path = config_path or get_config_path(model_path) + if model_path.startswith("http"): + if not validate_url(model_path): + raise ValueError(f"Invalid URL domain: {model_path}") model_path = NosTTSPlugin.download(model_path) + if config_path.startswith("http"): + if not validate_url(config_path): + raise ValueError(f"Invalid URL domain: {config_path}") config_path = NosTTSPlugin.download(config_path) + + if not os.path.exists(config_path): + raise FileNotFoundError(f"Config file not found: {config_path}")
Line range hint
73-79: Improve path handling robustnessThe current string replacement approach for determining config path is brittle. Consider using proper path manipulation functions.
Here's a more robust implementation:
- config_path = config_path or model_path.replace("celtia.pth", "config.json").replace("sabela.pth", "config.json") + def get_config_path(model_path: str) -> str: + """Get config path based on model path using proper path manipulation""" + if any(model in model_path for model in ["celtia.pth", "sabela.pth"]): + return os.path.join(os.path.dirname(model_path), "config.json") + return model_path.rsplit(".", 1)[0] + "_config.json" + + config_path = config_path or get_config_path(model_path)
Line range hint
73-83: Add logging for configuration path resolutionTo aid in debugging and monitoring, consider adding logging statements to track configuration path resolution.
+ LOG.debug(f"Resolving config path for model: {model_path}") config_path = config_path or model_path.replace("celtia.pth", "config.json").replace("sabela.pth", "config.json") + LOG.info(f"Using config path: {config_path}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
ovos_tts_plugin_nos/__init__.py(1 hunks)
🔇 Additional comments (1)
ovos_tts_plugin_nos/__init__.py (1)
73-73: Verify that both models can safely share the same config file
The change consolidates config files for both models (celtia.pth and sabela.pth) into a single config.json. Please ensure that both models are compatible with the same configuration settings.
✅ Verification successful
Let me gather more information about the config file usage and model configurations.
Let me check for any model-specific imports or dependencies that might indicate different configuration requirements.
Based on the gathered information, I can now provide the final response:
Sharing config.json between models is safe and intentional
The models can safely share the same config.json because:
- Both models are VITS-based but handle text differently (celtia uses graphemes, sabela uses phonemes)
- The model-specific processing is handled in the code:
- Sabela voice uses phonemization through Cotovia
- Celtia voice uses direct grapheme input
- The config.json is used only for basic model initialization in the Synthesizer class
- The README confirms both voices are supported through the same plugin configuration structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if models have different config requirements
for model in "celtia.pth" "sabela.pth"; do
echo "Checking config for $model:"
rg -A 10 -B 10 "$model.*config"
done
Length of output: 3367
Script:
#!/bin/bash
# Check for model-specific configurations or settings
echo "Checking model configurations:"
rg -l "config" --type py
echo -e "\nChecking Synthesizer usage and configuration:"
ast-grep --pattern 'class Synthesizer {
$$$
}'
echo -e "\nChecking config file content or structure references:"
rg -A 5 "config.*json"
Length of output: 953
Script:
#!/bin/bash
# Check for model imports and dependencies
echo "Checking imports and model-related code:"
rg -A 5 "^from|^import" ovos_tts_plugin_nos/__init__.py
echo -e "\nChecking model initialization or setup:"
rg -A 10 "def __init__" ovos_tts_plugin_nos/__init__.py
echo -e "\nChecking for any model-specific constants or settings:"
rg -B 5 -A 5 "CELTIA|SABELA" --ignore-case
Length of output: 5853
files renamed to just config.json
Summary by CodeRabbit
New Features
Bug Fixes